feat(podGC): add Workflow-level `DeleteDelayDuration` #11325

agilgur5 · 2023-07-10T23:23:06Z

Motivation

previously, this could only be set globally for the entire controller
now, users can configure this per Workflow as needed / wanted
- and can override the global setting as desired
- in particular, this matches how strategy etc can already be set per Workflow
  - so users have a single place to configure all options of podGC
in the future (I assume after some deprecation window), the global setting could be removed
- workflowDefaults can achieve the same thing now

Resolves #9894

I took a bit of a different approach from there; with more explicit and consistent naming and typing
- Using metav1.Duration was more consistent with the global and simplified the logic as well
- Added more comments and more consistent comments as well

Modifications

add DeleteDelayDuration to Workflow-level podGC
- add to workflow_types.go, then add logic to use this in pod_cleanup.go
- add to PodGC examples as well
- +codegen

Verification

So pod_cleanup_test.go actually has no unit test for the whole queuePodsForCleanup() function, so I couldn't just add a unit there. That function could require some mocking to properly test, so I didn't introduce a whole new test suite for that at this point.

Notes to Reviewers

Main issue I have is with the codegen for fields.md, where it explains metav1.Duration a bit misleadingly 😕

Everywhere else correctly shows that it boils down to just a string as well, but not this generated section 😕
See in-line comment about this

Related, durations seem a bit inconsistently typed in the codebase and spec. Some are ints, some are strings, and others are Durations.

Not sure if there's a reason for that? I would guess there is some history?

agilgur5 · 2023-07-10T23:30:18Z

docs/fields.md

+## Duration
+
+Duration is a wrapper around time.Duration which supports correctmarshaling to YAML and JSON. In particular, it marshals into strings, whichcan be used as map keys in json.
+
+<details>
+<summary>Examples with this field (click to open)</summary>
+<br>
+
+- [`pod-gc-strategy-with-label-selector.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/pod-gc-strategy-with-label-selector.yaml)
+
+- [`pod-gc-strategy.yaml`](https://github.com/argoproj/argo-workflows/blob/master/examples/pod-gc-strategy.yaml)
+</details>
+
+### Fields
+| Field Name | Field Type | Description   |
+|:----------:|:----------:|---------------|
+|`duration`|`string`|_No description available_|
+


this is the part I find issues with from the codegen. It's missing spaces (between multi-line comments in the Go), for one thing, but also it confusingly lists duration as a sub-field. That is correct for the Go type itself, but it marshals to a string, so the configuration is just a plain string (as the CRDs show).

This doc makes me think I would do:

podGC: deleteDelayDuration: duration: 30s

instead of just:

podGC: deleteDelayDuration: 30s

With the second one being how it actually is.

Is there a way to override this or something? Casting somewhere? Not sure

~~Well, I was able to fix the spacing by modifying this line in docgen.go, but while fixing this it ends up adding extraneous spaces in several places~~ 😕
~~The resulting diff for fields.md ends up quite large as well, so I didn't even commit that change~~

EDIT: was more or less able to fix the spacing issue with another ReplaceAll of multi-space to single space, but still has a big diff not limited to this field. and also doesn't resolve the confusing description here

- previously, this could only be set globally for the entire controller - now, users can configure this per Workflow as needed / wanted - and can override the global setting as desired - in particular, this matches how `strategy` etc can already be set per Workflow - so users have a single place to configure all options of `podGC` - in the future (I assume after some deprecation window), the global setting could be removed - `workflowDefaults` can achieve the same thing now Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

this one feels the most potentially problematic, so just split it into its own commit - though it's still generated, so still impacted by the other commits of this PR Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

- no more words or sentences joined together with no spaces in between at least - but also reduced spaces and increased spaces here and there as a side-effect Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

terrytangyuan

@juliev0 Could you help review this?

agilgur5 · 2023-07-12T14:41:23Z

Not sure how codegen is failing, I actually haven't been able to reproduce that locally, even after making sure my deps were updated and re-running several times. Can manually fix that, but odd that my local install is not producing the same output

juliev0 · 2023-07-12T19:13:38Z

Not sure how codegen is failing, I actually haven't been able to reproduce that locally, even after making sure my deps were updated and re-running several times. Can manually fix that, but odd that my local install is not producing the same output

I wonder if it could be running a different version of Go from what you are? looks like this is using 1.20: https://github.com/argoproj/argo-workflows/blob/master/.github/workflows/ci-build.yaml#L205

juliev0 · 2023-07-12T19:30:02Z

@agilgur5 Looks good. Once you get the CI all passing, let me know and I can approve.

agilgur5 · 2023-07-13T01:40:49Z

I wonder if it could be running a different version of Go from what you are? looks like this is using 1.20: https://github.com/argoproj/argo-workflows/blob/master/.github/workflows/ci-build.yaml#L205

So I'm using the dev container on my current machine, which is also using Go 1.20 😅
Confirmed that too:

vscode ➜ ~/go/src/github.com/argoproj/argo-workflows (feat-workflow-podgc-delay) $ go version
go version go1.20.5 linux/amd64

Strange. Might try rebuilding the dev container from scratch 🤷

- for some reason, I needed a fresh build of the devcontainer to get this to reproduce on my local machine - works now 🤷 Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

agilgur5 · 2023-07-13T03:14:00Z

Might try rebuilding the dev container from scratch 🤷

fresh build solved it 🤷

@agilgur5 Looks good. Once you get the CI all passing, let me know and I can approve.

@juliev0 should be good to go now!

Signed-off-by: Anton Gilgur <agilgur5@gmail.com> Signed-off-by: Tim Collins <tim@thecollins.team>

agilgur5 requested review from jessesuen and alexec as code owners July 10, 2023 23:23

agilgur5 force-pushed the feat-workflow-podgc-delay branch from 7424dc9 to d40fc45 Compare July 10, 2023 23:26

agilgur5 commented Jul 10, 2023

View reviewed changes

agilgur5 mentioned this pull request Jul 10, 2023

chore: mark more files as generated #11321

Merged

agilgur5 added 4 commits July 11, 2023 23:06

codegen (most)

04d2cd9

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

add'l codegen for docs/fields.md

1fc0efd

this one feels the most potentially problematic, so just split it into its own commit - though it's still generated, so still impacted by the other commits of this PR Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

improve docgen spacing a bit

da88e30

- no more words or sentences joined together with no spaces in between at least - but also reduced spaces and increased spaces here and there as a side-effect Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

terrytangyuan reviewed Jul 12, 2023

View reviewed changes

add'l codecov

e249623

- for some reason, I needed a fresh build of the devcontainer to get this to reproduce on my local machine - works now 🤷 Signed-off-by: Anton Gilgur <agilgur5@gmail.com>

agilgur5 force-pushed the feat-workflow-podgc-delay branch from 1152f7d to e249623 Compare July 13, 2023 02:41

agilgur5 requested a review from sarabala1979 as a code owner July 13, 2023 02:41

juliev0 approved these changes Jul 13, 2023

View reviewed changes

juliev0 merged commit 383bb6b into argoproj:master Jul 13, 2023
23 checks passed

tico24 pushed a commit to tico24/argo-workflows that referenced this pull request Jul 13, 2023

feat(podGC): add Workflow-level DeleteDelayDuration (argoproj#11325)

a78036d

Signed-off-by: Anton Gilgur <agilgur5@gmail.com> Signed-off-by: Tim Collins <tim@thecollins.team>

agilgur5 deleted the feat-workflow-podgc-delay branch July 13, 2023 14:13

agilgur5 mentioned this pull request Jul 13, 2023

docs: add GC details to Cost Optimization #11293

Merged

This was referenced Jul 25, 2023

podGC.deleteDelayDuration not working #11412

Closed

docs(podGC): specify that deleteDelayDuration requires >=3.5 #11445

Merged

terrytangyuan mentioned this pull request Aug 2, 2023

Release v3.5 preparation #11381

Closed

agilgur5 added area/controller Controller issues, panics area/spec Changes to the workflow specification. labels Aug 27, 2023

agilgur5 mentioned this pull request Dec 21, 2023

Incorrect message on UI when podGC is enabled with deleteDelayDuration #12395

Closed

hobti01 mentioned this pull request Feb 6, 2024

deleteDelayDuration JSON schema and validation mismatch #12631

Closed

4 tasks

agilgur5 added the area/gc Garbage collection, such as TTLs, retentionPolicy, delays, and more label Feb 18, 2024

agilgur5 mentioned this pull request May 22, 2024

docs: update CHANGELOG.md for v3.4.17 #13043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(podGC): add Workflow-level `DeleteDelayDuration` #11325

feat(podGC): add Workflow-level `DeleteDelayDuration` #11325

agilgur5 commented Jul 10, 2023 •

edited

Loading

agilgur5 Jul 10, 2023 •

edited

Loading

agilgur5 Jul 10, 2023 •

edited

Loading

terrytangyuan left a comment

agilgur5 commented Jul 12, 2023

juliev0 commented Jul 12, 2023 •

edited

Loading

juliev0 commented Jul 12, 2023

agilgur5 commented Jul 13, 2023

agilgur5 commented Jul 13, 2023

feat(podGC): add Workflow-level DeleteDelayDuration #11325

feat(podGC): add Workflow-level DeleteDelayDuration #11325

Conversation

agilgur5 commented Jul 10, 2023 • edited Loading

Motivation

Modifications

Verification

Notes to Reviewers

agilgur5 Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

agilgur5 Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

terrytangyuan left a comment

Choose a reason for hiding this comment

agilgur5 commented Jul 12, 2023

juliev0 commented Jul 12, 2023 • edited Loading

juliev0 commented Jul 12, 2023

agilgur5 commented Jul 13, 2023

agilgur5 commented Jul 13, 2023

feat(podGC): add Workflow-level `DeleteDelayDuration` #11325

feat(podGC): add Workflow-level `DeleteDelayDuration` #11325

agilgur5 commented Jul 10, 2023 •

edited

Loading

agilgur5 Jul 10, 2023 •

edited

Loading

agilgur5 Jul 10, 2023 •

edited

Loading

juliev0 commented Jul 12, 2023 •

edited

Loading